CODE REVIEW

The removal of the comment "TODO: create custom exceptions" suggests you might have addressed custom exception handling elsewhere. However, it's crucial to ensure that exception handling, especially custom exceptions, is implemented for robust error management. Custom exceptions could improve clarity and maintainability.

Consider defining exceptions like LoginError:

class LoginError(Exception):
    """Exception raised for errors during login."""
    pass

And using it in your login method:

def login(self) -> None:
    try:
        # login logic here
        pass
    except Exception as e:
        raise LoginError("Failed to log in") from e

CODE REVIEW

The refactoring into a UI class or module is a great idea for code organization and readability. However, consider implementing type hinting for list[Task] more explicitly if Task is a custom class. Use from typing import List and define the type of elements within the list for improved clarity.

from typing import List, Optional
from your_project.task import Task  # Assuming Task is a custom class

class UITaskManager:
    @staticmethod
    def print_task_list(
        task_list: List[Task], filter: Optional[str] = None, limit: Optional[int] = None
    ) -> None:
        # Implementation here

This change facilitates readability and future maintenance by clarifying that task_list is a list of Task instances.

CODE REVIEW

The removed TODO comment suggests implementing functionality to read an existing config file for default values. It's crucial to provide a fallback or initialization from existing configurations to enhance user experience and prevent unnecessary reconfigurations.

Consider using a function to attempt reading an existing config file, and if it exists, load these values as defaults:

import json

def read_existing_config() -> dict[str, str]:
    try:
        with open('config.json', 'r') as file:
            return json.load(file)
    except FileNotFoundError:
        return {}

def create_config() -> dict[str, str]:
    existing_config = read_existing_config()
    username = input("Your tmc.mooc.fi username: ") or existing_config.get('username', '')
    password = getpass("Your tmc.mooc.fi password: ") or existing_config.get('password', '')
    config = {

This approach improves the function by providing a smoother setup or re-setup experience.

CODE REVIEW

This change suggests an intent to replace argparse with click but hasn't been implemented yet. While argparse is a robust option for command-line interface creation, click can offer a more straightforward syntax and automatic help page generation. Since the code changes are minimal and the transition to click isn't shown, I recommend:

1. Implement click to see the benefits of cleaner syntax and easier command line options handling.

Example with click:

import click

@click.command()
@click.option('--username', help='tmc.mooc.fi username')
def main(username):
    click.echo(f"Hello, {username}")

2. Complete the TODO before merging.

CODE REVIEW

The change replacing specific TODO comments with a link to an issue is a good practice for tracking, but ensure the issue contains clear instructions or code examples on the solution. For the write_config function, handling file overwrite by asking for user permission is crucial for user experience and preventing accidental data loss.

Instead of a placeholder or generic comments, consider implementing an interactive prompt or a mechanism to backup before overwriting, e.g.:

from shutil import copy

if file_path.exists():
    overwrite = input(f"{file_path} exists. Overwrite? (y/n): ").strip().lower()
    if overwrite == 'y':
        backup_path = f"{file_path}.bak"
        copy(file_path, backup_path)
        print(f"Backup created at {backup_path}")
    else:
        print("Operation cancelled.")
        return

For read_config, indeed adding try/except around file opening and loading is important for robustness:

try:
    with open(file_path, 'r') as f:
        config = json.load(f)
except (FileNotFoundError, json.JSONDecodeError):
    return {}

CODE REVIEW

It seems you intend to implement a feature to submit tasks. A few suggestions to improve the current changes:

1. Docstring Improvement: Ensure your docstring accurately describes the function, including its parameters and what it returns or accomplishes. Since the function is not yet implemented, consider noting that it's a work in progress.

2. Use of TODOs: It's good you've noted the need to implement the function but ensure in subsequent commits the implementation follows soon after for maintainability.

3. Function Implementation Placeholder: If the function is incomplete, perhaps raise a NotImplementedError as a placeholder to avoid silent failures if the function is mistakenly called.

Example:

def submit_task(task_id: str, filename: str) -> None:
    """
    Submit file to the submit form for a given task_id.
    
    Parameters:
    - task_id: The ID of the task to submit to.
    - filename: The name of the file to submit.
    
    Returns:
    None. Submits the task and handles the submission process.
    """
    raise NotImplementedError("Function submit_task is not yet implemented.")

CODE REVIEW

Your changes indicate the removal of several TODO comments and the import of argparse, dataclasses, and enum. It seems like you're preparing for some argument parsing and structured data handling. A few suggestions:

1. Comment Removal: If these TODOs were removed because they've been implemented, consider ensuring there's clear documentation or comments in place where each task was addressed.
2. Data Handling: Using dataclass and Enum is a good choice for clean and efficient data representation. Ensure each dataclass and Enum is well-defined and includes type hints for clarity.

Example:

from enum import Enum
from dataclasses import dataclass

class Status(Enum):
    TODO = "TODO"
    IN_PROGRESS = "IN_PROGRESS"
    DONE = "DONE"

@dataclass
class Exercise:
    name: str
    status: Status
    week: int = field(default=0)
    deadline: str = field(default="")

3. Argument Parsing: It's great you're setting up for command-line interaction. Make sure to define clear, user-friendly arguments with argparse, including help descriptions for each argument.

Remember, well-commented code and clear structure are key to maintainability and overall project health.

CODE REVIEW

The implementation of parse_task is a good start. However, there are a few points to consider:

1. The removal of TODO comments without apparent implementation hints at unfinished work or missing functionality. Ensure all intended features are implemented or tracked elsewhere.

2. For readability and future maintenance, adding type annotations for the return type of htmlement.fromstring(html) would be helpful, assuming root has a specific type that could be annotated.

3. It might be beneficial to consider error handling if .find does not locate the element, preventing potential runtime errors.

Here's a potential improvement:

from typing import Optional  # If not already imported

def parse_task(html: AnyStr) -> Optional[Task]:
    try:
        root = htmlement.fromstring(html)  # Assuming root's type can be more specifically annotated
        task_link_element = root.find('.//div[@class="nav sidebar"]/a')
        if not task_link_element:
            return None  # Or handle absence in another way
        # Process task_link_element into a Task object...
    except Exception as e:
        # Properly handle or log the exception
        return None

This adjustment adds robustness against potential parsing failures and clarifies the handling of cases where elements are not found.

CODE REVIEW

The code correctly identifies the need for organizing the parse_task_list function into a client class or module for better structure. This change encapsulates functionality and could enable easy maintenance and scalability.

However, ensure AnyStr from typing is used appropriately to accept both str and bytes types. If only one is expected, use str or bytes instead for clarity.

Consider introducing a data class for Task to ensure the returned list has a well-defined structure, improving code readability and safety.

Example improvement:

from typing import List
from dataclasses import dataclass

@dataclass
class Task:
    # Assuming tasks attributes here
    id: int
    status: str

class TaskParser:
    @staticmethod
    def parse_task_list(html: str) -> List[Task]:
        content_element = htmlement.fromstring(html).find('.//div[@class="content"]')
        # Parsing logic here
        return [Task(id=1, status='Complete')]  # Example return